Add DatasetsV2Client (WIP) - #1
Open
Hellebore wants to merge 1 commit into
Open
Conversation
Author
|
This is a benchmark review for experiment This pull request was cloned from Experiment configurationreview_config:
# User configuration for the review
# - benchmark - use the user config from the benchmark reviews
# - <value> - use the value directly
user_review_config:
enable_ai_review: true
enable_rule_comments: false
enable_complexity_comments: benchmark
enable_security_comments: benchmark
enable_tests_comments: benchmark
enable_comment_suggestions: benchmark
enable_pull_request_summary: benchmark
enable_review_guide: benchmark
enable_approvals: false
base_branches: [base-sha.*]
ai_review_config:
# The model responses to use for the experiment
# - benchmark - use the model responses from the benchmark reviews
# - llm - call the language model to generate responses
model_responses:
comments_model: benchmark
comment_validation_model: benchmark
comment_suggestion_model: benchmark
complexity_model: benchmark
security_model: benchmark
tests_model: benchmark
pull_request_summary_model: benchmark
review_guide_model: benchmark
overall_comments_model: benchmark
# The pull request dataset to run the experiment on
pull_request_dataset:
# CodeRabbit
- https://github.com/neerajkumar161/node-coveralls-integration/pull/5
- https://github.com/gunner95/vertx-rest/pull/1
- https://github.com/Altinn/altinn-access-management-frontend/pull/1427
- https://github.com/theMr17/github-notifier/pull/14
- https://github.com/bearycool11/AI_memory_Loops/pull/142
# Greptile
- https://github.com/gumloop/guMCP/pull/119
- https://github.com/autoblocksai/python-sdk/pull/335
- https://github.com/grepdemos/ImageSharp/pull/6
- https://github.com/grepdemos/server/pull/61
- https://github.com/websentry-ai/pipelines/pull/25
# Graphite
- https://github.com/KittyCAD/modeling-app/pull/6648
- https://github.com/KittyCAD/modeling-app/pull/6628
- https://github.com/Varedis-Org/AI-Test-Repo/pull/2
- https://github.com/deeep-network/bedrock/pull/198
- https://github.com/Metta-AI/metta/pull/277
# Copilot
- https://github.com/hmcts/rpx-xui-webapp/pull/4438
- https://github.com/ganchdev/quez/pull/104
- https://github.com/xbcsmith/ymlfxr/pull/13
- https://github.com/tinapayy/B-1N1T/pull/36
- https://github.com/coder/devcontainer-features/pull/6
# Questions to ask to label the review comments
review_comment_labels: []
# - label: correct
# question: Is this comment correct?
# Benchmark reviews generated by running
# python -m scripts.experiment benchmark <experiment_name>
benchmark_reviews: []
|
Author
Reviewer's GuideThis pull request implements the File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Hellebore
commented
May 2, 2025
Hellebore
left a comment
Hellebore
left a comment
Author
There was a problem hiding this comment.
Hey @Hellebore - I've reviewed your changes - here's some feedback:
- Consider raising more specific exception types from
_make_requestinstead of a genericExceptionto allow for better error handling by the caller. - Ensure all dynamic parts of API request paths, including
app_slug, are consistently URL-encoded.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+79
to
+80
| if not response.ok: | ||
| raise Exception(f"HTTP Request Error: {method} {url} \"{response.status_code} {response.reason}\"") |
Author
There was a problem hiding this comment.
suggestion: Raise a more specific exception instead of a generic Exception.
Define and use a custom or specific exception so SDK clients can catch API errors precisely.
Suggested implementation:
import requests
class APIRequestError(Exception):
"""Exception raised when an API request fails."""
def __init__(self, method, url, status_code, reason):
message = f"HTTP Request Error: {method} {url} \"{status_code} {reason}\""
super().__init__(message)
self.method = method
self.url = url
self.status_code = status_code
self.reason = reason if not response.ok:
raise APIRequestError(method, url, response.status_code, response.reason)Make sure to update any exception handling logic in the SDK clients to catch APIRequestError instead of a generic Exception if needed.
Comment on lines
+27
to
+36
| @mock.patch('requests.request') | ||
| def test_list_datasets(mock_request): | ||
| """Test listing datasets""" | ||
| # Setup mock response | ||
| mock_response = mock.Mock() | ||
| mock_response.ok = True | ||
| mock_response.json.return_value = [ | ||
| { | ||
| "id": "dataset-id-1", | ||
| "external_id": "test-dataset-1", |
Author
There was a problem hiding this comment.
suggestion (testing): Missing test for API error response
Add a test for the error path by mocking mock_response.ok = False and mock_response.status_code = 500, then assert the expected exception is raised.
Suggested implementation:
@mock.patch('requests.request')
def test_list_datasets_error(mock_request):
"""Test listing datasets when API returns an error response"""
import pytest
# Setup mock response to simulate error case
mock_response = mock.Mock()
mock_response.ok = False
mock_response.status_code = 500
mock_response.text = "Internal Server Error"
mock_request.return_value = mock_response
client = DatasetsV2Client({
"api_key": "test-api-key",
"app_slug": "test-app"
})
# Replace Exception with the expected exception type as per your implementation
with pytest.raises(Exception):
client.list_datasets()Ensure that your DatasetsV2Client.list_datasets() method raises an exception when the response is not OK. Replace Exception with the specific exception if one is already defined.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Greptile Summary
This PR adds a comprehensive DatasetsV2Client implementation for the Autoblocks Python SDK, providing CRUD operations for datasets with schema versioning and validation support.
autoblocks/_impl/datasets_v2/__init__.pyneeds to be populated with proper re-exports to expose the package's public APIclient.pyshould enhance error handling and input validation, particularly around URL construction and special characterstest_client.pyshould be expanded to include error cases, timeout handling, and URL encoding validationdatasets_v2_example.pyThe implementation is well-structured with good type definitions and validation logic, but these improvements would make it more robust and production-ready.
Summary by Sourcery
Implement a comprehensive DatasetsV2Client for the Autoblocks Python SDK, providing robust CRUD operations for datasets with schema versioning, validation, and type-safe interactions.
New Features:
Enhancements:
Documentation:
Tests: